Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(package.json): reorder 'test' scripts to run 'lint' before 'format' #3019

Closed
wants to merge 1 commit into from

Conversation

sukvvon
Copy link
Contributor

@sukvvon sukvvon commented Feb 18, 2025

Summary

Check List

  • pnpm run fix for formatting and linting code and docs

Copy link

vercel bot commented Feb 18, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
zustand-demo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 18, 2025 6:24am

Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Copy link

pkg-pr-new bot commented Feb 18, 2025

Open in Stackblitzdemostarter

npm i https://pkg.pr.new/zustand@3019

commit: 8579bb7

@dai-shi
Copy link
Member

dai-shi commented Feb 18, 2025

I actually didn't notice when I reviewed #2997, but it seems to run scripts in parallel. https://pnpm.io/9.x/cli/run#running-multiple-scripts

How did #2997 solve it?

@sukvvon
Copy link
Contributor Author

sukvvon commented Feb 18, 2025

@dai-shi

I actually didn't notice when I reviewed #2997, but it seems to run scripts in parallel. https://pnpm.io/9.x/cli/run#running-multiple-scripts

How did #2997 solve it?

Because we agree with lint --fix catches and fixes code issues first, and prettier --write then formats the code for consistency.
I have confirmed that the script executes sequentially in the order it was written.

@dai-shi
Copy link
Member

dai-shi commented Feb 18, 2025

Isn't it like if eslint is slow, prettier finishes before eslint?

@sukvvon
Copy link
Contributor Author

sukvvon commented Feb 18, 2025

Isn't it like if eslint is slow, prettier finishes before eslint?

When I tested it, it executed sequentially.

https://pnpm.io/9.x/cli/run#running-multiple-scripts

You may run multiple scripts at the same time by using a regex instead of the script name.

In my opinion, although it is said to run simultaneously, it seems that the execution starts at the same time, but the actual execution happens sequentially.”

@dai-shi
Copy link
Member

dai-shi commented Feb 18, 2025

When I run pnpm run test, vitest run finishes at first on my local machine.

> pnpm run "/^test:.*/"

. test:format$ prettier "*.{js,json,md}" "{examples,src,tests,docs}/**/*.{js,…
└─ Done in 2.9s
. test:types$ tsc --noEmit
└─ Done in 2.6s
. test:lint$ eslint .
└─ Done in 4.8s
. test:spec$ vitest run
[7 lines collapsed]
│  ✓ |zustand| tests/persistAsync.test.tsx (17 tests) 212ms
│  ✓ |zustand| tests/vanilla/subscribe.test.tsx (8 tests) 7ms
│  ✓ |zustand| tests/vanilla/shallow.test.tsx (10 tests) 8ms
│  ✓ |zustand| tests/vanilla/basic.test.ts (9 tests) 51ms
│  ↓ |zustand| tests/ssr.test.tsx (2 tests | 2 skipped)
│  ✓ |zustand| tests/subscribe.test.tsx (1 test) 1ms
│  Test Files  11 passed | 1 skipped (12)
│       Tests  180 passed | 2 skipped (182)
│    Start at  16:08:34
│    Duration  2.07s (transform 713ms, setup 1.23s, collect 2.32s, tests 721m…
└─ Done in 2.3s

@sukvvon
Copy link
Contributor Author

sukvvon commented Feb 18, 2025

When I run pnpm run test, vitest run finishes at first on my local machine.

> pnpm run "/^test:.*/"

. test:format$ prettier "*.{js,json,md}" "{examples,src,tests,docs}/**/*.{js,…
└─ Done in 2.9s
. test:types$ tsc --noEmit
└─ Done in 2.6s
. test:lint$ eslint .
└─ Done in 4.8s
. test:spec$ vitest run
[7 lines collapsed]
│  ✓ |zustand| tests/persistAsync.test.tsx (17 tests) 212ms
│  ✓ |zustand| tests/vanilla/subscribe.test.tsx (8 tests) 7ms
│  ✓ |zustand| tests/vanilla/shallow.test.tsx (10 tests) 8ms
│  ✓ |zustand| tests/vanilla/basic.test.ts (9 tests) 51ms
│  ↓ |zustand| tests/ssr.test.tsx (2 tests | 2 skipped)
│  ✓ |zustand| tests/subscribe.test.tsx (1 test) 1ms
│  Test Files  11 passed | 1 skipped (12)
│       Tests  180 passed | 2 skipped (182)
│    Start at  16:08:34
│    Duration  2.07s (transform 713ms, setup 1.23s, collect 2.32s, tests 721m…
└─ Done in 2.3s

@dai-shi I think I misunderstood. You’re right. In that case, the purpose of this PR would be to align with the format.

@sukvvon
Copy link
Contributor Author

sukvvon commented Feb 18, 2025

"fix": "eslint . --fix && prettier \"*.{js,json,md}\" \"{examples,src,tests,docs}/**/*.{js,jsx,ts,tsx,md,mdx}\" --write",

In fix script, we have to run sequentially, so we can fix like that

@dai-shi
Copy link
Member

dai-shi commented Feb 18, 2025

As I said in #2997, I don't worry about it too much.
If it doesn't work at all, let's revert it. If it help a bit, let's leave it for now. But we don't know the future.

I don't think changing the order of test scripts is necessary. (As we might possibly revert #2997 in the future.)

@dai-shi dai-shi closed this Feb 18, 2025
@sukvvon
Copy link
Contributor Author

sukvvon commented Feb 18, 2025

"fix": "eslint . --fix && prettier \"*.{js,json,md}\" \"{examples,src,tests,docs}/**/*.{js,jsx,ts,tsx,md,mdx}\" --write",

In fix script, we have to run sequentially, so we can fix like that

"fix": "pnpm run fix:lint && pnpm run fix:format",
"fix:lint": "eslint . --fix",
"fix:format": "prettier \"*.{js,json,md}\" \"src/**/*.{js,jsx,ts,tsx}\" --write",

@dai-shi Do you mean that if I don’t modify it as suggested, there is a potential for conflicts, but since it hasn’t happened yet, it’s fine to leave it as is for now?

@dai-shi
Copy link
Member

dai-shi commented Feb 18, 2025

It feels to me that making it sequential is less consistent. In either case without it, there can be a conflict, but it's expected and it feels very rare (so, yes, unless someone complains it, we can leave it). I'm fine to revert #2997 now for consistency, but since you said it works for your case, leaving it is okay too.

Hmm, but wait, can you clarify why #2997 fixes your case?

@sukvvon
Copy link
Contributor Author

sukvvon commented Feb 18, 2025

Hmm, but wait, can you clarify why #2997 fixes your case?

it was my misunderstood result

"fix": "pnpm run fix:lint && pnpm run fix:format",
"fix:lint": "eslint . --fix",
"fix:format": "prettier \"*.{js,json,md}\" \"src/**/*.{js,jsx,ts,tsx}\" --write",

So i wanna fix my misunderstood result like that in this PR

@dai-shi
Copy link
Member

dai-shi commented Feb 18, 2025

Okay, then, feel free to send new PRs for sequential execution. I'm not so strongly against it anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants